walletrpc: add XCreateAccount for wallet-derived accounts - #11065

Blocking: the caveat is right but the recovery procedure given here does not work, in two separate ways. Someone following it exactly will rescan, find nothing, and reasonably conclude the coins are unrecoverable.

1. The account name is not part of the derivation — the account index is. ScopedKeyManager.NewAccount assigns fetchLastAccount(ns, &s.scope) + 1 (waddrmgr/scoped_manager.go:1625-1633), and NewAccountWatchingOnly shares that same counter (:1742). So backing up the name buys you nothing: what has to be reproduced is the key scope and the account index, which means re-creating every account in that scope — including any ImportAccount ones — in the original order, so the counter lands on the same value. Two accounts recreated in the wrong order gives you two accounts with each other's addresses.

2. Re-creating the account is not sufficient, because a rescan only looks for addresses already in the wallet DB. Wallet.activeData feeds the rescan from Manager.ForEachRelevantActiveAddress (waddrmgr/manager.go:827), i.e. addresses that have already been derived. A freshly created account has ExternalKeyCount == 0, so a rescan over it searches for zero addresses. The user also has to re-derive at least as many addresses as were previously used — NextAddr in a loop against the recreated account — before triggering the rescan.

So the accurate procedure is roughly: record the key scope, the account index, and the number of addresses issued; on restore, re-create accounts in that scope in original order until the index matches, call NextAddr at least that many times, then start with --reset-wallet-transactions. That is a genuinely recoverable situation, which is a much better story than the current text tells — worth spelling out rather than leaving as a flat "will not rediscover".

It is also worth citing the reason it is this way, since it bounds how permanent the caveat is: btcwallet's RecoveryManager.Resurrect hardcodes waddrmgr.DefaultAccountNum (wallet/recovery.go:72) and carries an explicit // TODO(conner): rescan for all created accounts if we allow users to use non-default address — which this RPC is precisely the trigger for. Might be worth opening that btcwallet issue alongside the base.Interface one.

"later calls such as NextAddr must ask for the same address type" is not followable for one of the three types you accept. NewAddress/NextAddr take lnrpc.AddressType, which has no HYBRID_* member — it is {WITNESS_PUBKEY_HASH, NESTED_PUBKEY_HASH, TAPROOT_PUBKEY, ...} (lnrpc/lightning.proto:1193). An account created as HYBRID_NESTED_WITNESS_PUBKEY_HASH has to be addressed with NESTED_PUBKEY_HASH, since rpcserver.go:1698 maps that to lnwallet.NestedWitnessPubKey → KeyScopeBIP0049Plus in keyScopeForAccountAddr. Suggest naming the mapping explicitly rather than saying "the same address type".

Separately, and the reason this caveat keeps costing people time: the failure mode is a bare not-found. keyScopeForAccountAddr (lnwallet/btcwallet/btcwallet.go:470) calls AccountNumber(addrKeyScope, accountName) and returns waddrmgr's account name 'x' not found verbatim — no hint that the account exists, just under a different scope. Given this same edge has now had to be worked around independently in lightninglabs/wavelength#1140 and lightninglabs/lumos#786 and documented a third time here, the error message is arguably the actual bug. A cheap fix in the not-found branch: fall back to lookupFirstCustomAccount, and if the name does resolve under another scope, say so — account "x" exists under key scope %v, not %v; request address type %v instead. Non-blocking, but it would retire the footgun instead of documenting it a fourth time.

Verified the reasoning here: ListAccounts(name, nil) on the custom-name path and lookupFirstCustomAccount both range over waddrmgr.DefaultKeyScopes, so the check covers exactly the set that could later become ambiguous, and a scope added upstream propagates to both without touching this code. Good.

One gap worth a line of acknowledgement: this check and the NextAccount below are separate db transactions, and btcwallet's own duplicate check (newAccount → lookupAccount, scoped_manager.go:1654) is per-scope. Two concurrent CreateAccount calls with the same name under different scopes therefore both pass here and both succeed, producing exactly the ambiguity this guard exists to prevent. Realistically an operator does not race themselves, so I would not restructure for it — but a NOTE: saying the cross-scope invariant is best-effort and not atomic would stop the next reader assuming it is enforced.

Checked the degradation path since it is the thing an interface assertion usually gets wrong: b.wallet is a base.Interface, the concrete *wallet.Wallet satisfies NextAccount (btcwallet@v0.18.0 wallet/wallet.go:2090), and anything else gets this typed error rather than a nil-deref. The %T is a nice touch for diagnosing it. No objection to landing it as-is.

On the choice itself, answering the question in the description: the replace argument is correct and is the deciding factor — Go ignores replace directives in non-main modules, so forking btcwallet here would be a no-op for every consumer of lnd and each would have to carry its own copy. Widening base.Interface upstream is still the right end state; open that PR in parallel and drop this on the next btcwallet bump, which is what the comment already commits to.

Confirmed this is needed and that it is the only place that needs it — RPCKeyRing and lnwallet.LightningWallet (lnwallet/wallet.go:419) are the only two embedders of WalletController, and under remote signing config_builder.go:891 installs rpcKeyRing as LightningWallet.WalletController, so cc.Wallet.CreateAccount lands here. Without it you would indeed have gotten ErrWatchingOnly from scoped_manager.go:1611, which is exactly the unhelpful bare error you describe. Error text points at the right alternative too.

Only nit: this returns a plain error, so it surfaces over gRPC as Unknown. status.Error(codes.Unimplemented, ...) would let callers branch on it — mildly useful for the downstream consumers, which have to decide whether to fall back to the default account. Take it or leave it.

The description covers the permanence of the address type but says nothing about the recovery caveat, which is the part that can actually cost someone money. I do not think this needs an interactive y/N — that gets in the way of scripted operator setup and people click through prompts anyway — but the caveat should at least appear here, and ideally be echoed to stderr after a successful create, along the lines of: funds in this account are not found by a seed-only restore; record the account's key scope and index and see before depositing.

That also makes the lncli output the place the operator learns the index, which per the note on walletkit.proto is the thing they actually need to have written down. printRespJSON(resp) already returns the Account with its derivation_path, so it is right there — just worth pointing at.

Wondering if we should expose the ability to set an explicit key family/account here. As in other codebases we may already have a region carved out.

If say keyfam 77 is taken, and we allow it to be specified, what would be the resulting behavior?

Good question, and the answer is reassuring: key families and accounts are different namespaces, so a taken keyfam can't collide here.

lnd's own keys live under the BIP-0043 purpose 1017 scope (keychain.BIP0043Purpose), where the account slot is what carries the KeyFamily — so keyfam 77 is m/1017'/<coin>'/77'. CreateAccount only ever writes into the BIP-49Plus/84/86 scopes (whichever the requested address type maps to), so an account created here is m/86'/0'/N' and can never land on m/1017'/…/77'. They can't overlap, and ListAccounts doesn't surface 1017 accounts as named accounts either.

On exposing the index explicitly: I'd like to, but btcwallet doesn't currently allow it. ScopedKeyManager.NewAccount assigns fetchLastAccount + 1 unconditionally — there's no "create at index N" entry point, and the same counter is shared with ImportAccount. So accepting an explicit index would mean either a btcwallet change or creating-and-discarding accounts until the counter lands where you asked, which seems worse than not offering it.

That matters more than it first looks, because the index — not the name — is what a seed-only recovery needs (the recovery scan only rederives account 0, so restoring a created account means reproducing its scope and index). The response does return it: Account.derivation_path carries the full path, and lncli now points at it after a successful create for exactly that reason.

If you'd like an explicit index, I'm happy to open the btcwallet PR to add a NewAccountAtIndex-style entry point and follow up here — it'd pair naturally with the base.Interface widening this PR already wants.

A custom account lives in exactly one key scope

Meaning the BIP 86/84 scope?

Yes — one of BIP-0049Plus, BIP-0084 or BIP-0086, whichever the address type at creation mapped to, and fixed for the account's lifetime. I've named the three in the comment rather than leaving it implicit.

Nice lil assertion trick here

There's a potential race here. List then next isn't under the same db transaction, so another concurrent caller can win over.

We don't have anything exposed today to handle this in a single unit, so perhaps a mutex is the best we can do here.

Agreed, and done — added a createAccountMtx held across both the duplicate check and NextAccount. As you say there's nothing that does the two in one db transaction today, so a mutex is what's available; the comment says so explicitly and notes it only serialises callers within this process (lnd is the sole writer of its own wallet).

Also added TestCreateAccountSerialisesCallers, which fails without the mutex — the wallet fake reports the greatest number of callers it ever saw inside the check-then-create section, so it asserts serialisation directly rather than trying to lose a race by chance. Worth flagging because my first attempt at that test still passed with the mutex removed, which made it worthless.

The mutex closes the CreateAccount-vs-CreateAccount window, but the same cross-scope invariant is still racy against ImportAccount.

BtcWallet.ImportAccount (btcwallet.go:936-950 on this branch) does the identical check-then-act — b.ListAccounts(name, nil) followed by b.wallet.ImportAccount(...) — and takes no lock. Since gRPC handlers run concurrently, CreateAccount("foo", BIP0086) and ImportAccount("foo", ...) can both pass their duplicate checks and both succeed, leaving one name under two key scopes. That's exactly the ambiguity this mutex exists to prevent, just reached from the other side.

Suggest renaming it to accountMtx and taking it in ImportAccount too, wrapping the ListAccounts check through both the dry-run and non-dry-run branches (the dry run also consults ListAccounts before deciding).

Worth noting the ImportAccount half is pre-existing on master, so it could also be a follow-up — but since this PR is the one establishing the invariant, it seems natural to close it here.

Good catch — closed. Renamed to accountMtx and ImportAccount now takes it too, held across its ListAccounts check and both the dry-run and non-dry-run branches. You're right that it's the same invariant reached from the other side, and that it was worth doing here since this is the PR that establishes the invariant.

Nit: the empty-name check sits above Lock() but this reserved-name check is below it, even though it's a pure string comparison that touches no shared state. Would read better with all the input validation grouped before the critical section.

Moved — the reserved-name check now sits with the empty-name check above Lock(), so all the pure input validation is grouped before the critical section.

Heads up that this converts a typed waddrmgr.ManagerError{ErrAccountNotFound} into an untyped fmt.Errorf on the "exists under a different scope" path, which is a silent contract change for NewAddress/LastUnusedAddress.

Nothing outside lnwallet/btcwallet inspects that code today, so it's safe as-is — and %w wouldn't help anyway since waddrmgr.IsError type-asserts rather than unwrapping. Just worth a line in the commit message so it's not a surprise later.

Noted in the commit message, thanks. It now says plainly that keyScopeForAccountAddr reports a wrong-scope lookup as an untyped error naming the scope the account does live in, rather than passing through waddrmgr's typed ErrAccountNotFound, and why that's safe today.

Two things about returning a gRPC status from the wallet layer here:

1. The file's convention for unsupported-in-remote-signing ops is a plain error — see ErrRemoteSigningPrivateKeyNotAvailable at the top of this file.
2. codes.Unimplemented has a specific meaning in gRPC: "the server does not implement this method". Clients and version-negotiation logic routinely read it as "peer is too old" and fall back accordingly. Here the method is implemented — it's the node's configuration that forbids it. codes.FailedPrecondition conveys that accurately, or just a plain error to match the rest of the file.

Agreed on both counts, and the second is the stronger argument — the method is implemented, it's the configuration that forbids it, so Unimplemented would actively mislead version negotiation. Switched to a plain error following the file's convention: a package-level ErrRemoteSigningAccountCreation alongside ErrRemoteSigningPrivateKeyNotAvailable.

The handler drops the context, which is consistent with the other WalletKit handlers — but this one mutates persistent state, so the consequence is a bit sharper: a client that hits its deadline or cancels still gets the account created, and its retry then fails with "already exists" with no way to distinguish that from a genuine name clash.

Not worth plumbing a context through WalletController for, but a NOTE in the proto docs that the call isn't idempotent and that "already exists" may be the result of a retried-but-successful create would save someone a debugging session.

Added to the proto docs: the call is not idempotent, the account is created before the response is sent, so a cancelled or timed-out client may still have had it created and its retry will fail with "already exists" indistinguishably from a real clash — with a pointer to check ListAccounts before retrying.

enter()/exit() only instrument AccountPropertiesByName, so maxInFlight() == 1 proves the check is serialised — not that the check and the creation sit in the same critical section.

A lock that wrapped only the ListAccounts call would pass this test while leaving the actual bug intact, and check+create atomicity is the whole property the mutex exists for. Instrumenting NextAccount with the same enter()/exit() pair would close that.

You're right, and this is the more useful half of the property — the test as written would have passed a lock that covered only the lookup. NextAccount is now instrumented with the same enter()/exit() pair. I verified it both ways: it fails when the lock is narrowed to just the ListAccounts call, and passes with the lock spanning check and create.

Style: for i := range callers per the repo convention. The go func(i int) parameter is also unnecessary on Go >= 1.22 now that the loop var is per-iteration.

Done — for i := range callers, and dropped the now-unnecessary loop-variable parameter.

The comment says "the account still holds its funds minus fees", but only the default account is asserted here — the custom account's post-spend balance never gets checked.

That's the strongest post-condition in the test: it's what proves the spend came out of the account and the change went back into it rather than leaking to the default account. Worth an AssertWalletAccountBalance on createAccountName too (or trimming the claim from the comment).

Added rather than trimmed, since you're right that it's the strongest post-condition. The test now reads the account's own confirmed balance back from WalletBalance's per-account map and asserts it is below the funded amount (a fee was paid) but above funded-minus-a-fee-ceiling — which is what shows the inputs came from the account and the change returned to it, not to default.

parseAddrType maps np2wkh to NESTED_WITNESS_PUBKEY_HASH, which the server unconditionally rejects. The flag's own usage string already omits it, so the CLI accepts a value it knows will fail.

Rejecting it locally would give the user the better "use np2wkh-p2wkh instead" message without the round trip.

Done — lncli now rejects np2wkh locally with the "use np2wkh-p2wkh instead" message, before dialling.

Ideally we could cover different address types too.

Could we make the two address branches explicit in this recovery procedure? Account tracks external_key_count and internal_key_count separately, and NextAddr selects the branch with change (which defaults to false). Replaying a single aggregate count with the default therefore only recreates external addresses and can leave UTXOs on internal/change addresses invisible to the rescan—the likely location of most of the remaining balance after FundPsbt.

I think the actionable procedure should say to preserve both counters over the account lifetime, then call NextAddr(change=false) at least external_key_count times and NextAddr(change=true) at least internal_key_count times before rescanning. The lncli warning should reflect this too: recording only the derivation path at creation is not sufficient because both counters are zero then and grow as the account is used.

Since the documented manual reconstruction is currently the only recovery mitigation, could we cover that procedure end to end as well? I am not suggesting that this PR must implement automatic seed-only discovery—the X/acknowledgement gates already make that limitation explicit—but the claim that the funds remain manually recoverable is important enough to verify before users rely on it.

The useful regression test would create a preceding account so the target has a non-trivial index, fund an external address, then spend through FundPsbt so value remains on an internal/change address. Record the target xpub plus both branch counts; restore the same seed into a fresh wallet DB; recreate accounts until the same scope/index is reached and assert the xpub matches; replay NextAddr(change=false) and NextAddr(change=true) using their respective counts; rescan with --reset-wallet-transactions; and finally assert that both balances are found and can be spent. Leaving real value on the internal branch is the part that would catch the recovery issue above.